Skip to content

Optional first arg for Vector.record can be an ARTIFICIAL_CELL - #2858

Draft
nrnhines wants to merge 5 commits into
masterfrom
hines/record-artcell
Draft

Optional first arg for Vector.record can be an ARTIFICIAL_CELL#2858
nrnhines wants to merge 5 commits into
masterfrom
hines/record-artcell

Conversation

@nrnhines

@nrnhines nrnhines commented Apr 25, 2024

Copy link
Copy Markdown
Member

The existing logic of the if statement that generated the error message "Optional first arg is not a POINT_PROCESS" was incorrect.

[ ] add test
[ ] update documentation

The Vector.record also presently requires the existence of a Section. This seems unnecessary if the POINT_PROCESS or ARTIFICIAL_CELL arg is present. Section could disagree with POINT_PROCESS about which thread they are in.
It is not easy for the user to determine the thread of an ARTIFICIAL_CELL. Perhaps there should be a method for ARTIFICIAL_CELL and POINT_PROCESS which returns the thread id. Also, recording thread time and associating with a range variable is not obvious to the user. Finally, Vector.record(callable) could be useful.

In most cases, i.e. all range variables, aren't the POINT_PROCESS or Section hints unnecessary? Because the reference argument is a DataHandle. The hint is only needed when a reference, e.g. h._ref_t is not associated with (the desired) thread or cvode instance.

@azure-pipelines

Copy link
Copy Markdown

✔️ 0041a06 -> Azure artifacts URL

@codecov

codecov Bot commented Apr 25, 2024

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.07%. Comparing base (c18450a) to head (2a96b68).
⚠️ Report is 1181 commits behind head on master.

Files with missing lines Patch % Lines
src/nrniv/vrecord.cpp 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2858      +/-   ##
==========================================
- Coverage   67.07%   67.07%   -0.01%     
==========================================
  Files         569      569              
  Lines      111206   111207       +1     
==========================================
  Hits        74592    74592              
- Misses      36614    36615       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bbpbuildbot

This comment has been minimized.

@nrnhines
nrnhines marked this pull request as draft April 30, 2024 11:53
@pramodk

pramodk commented Apr 30, 2024

Copy link
Copy Markdown
Member

The CI was previously green here. I have re-launched CI on this branch just to see if the issue with recent latest failures is code related or runners environment. cc: @alkino

@sonarqubecloud

sonarqubecloud Bot commented May 2, 2024

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@bbpbuildbot

This comment has been minimized.

@azure-pipelines

Copy link
Copy Markdown

✔️ e10de94 -> Azure artifacts URL

@sonarqubecloud

Copy link
Copy Markdown

@azure-pipelines

Copy link
Copy Markdown

✔️ 2a96b68 -> Azure artifacts URL

@JCGoran JCGoran modified the milestones: Release v9.0, Release v9.1 Sep 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants